-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(inbound-filters): Add react-hydration-errors filter #45188
feat(inbound-filters): Add react-hydration-errors filter #45188
Conversation
src/sentry/ingest/test.json
Outdated
@@ -0,0 +1,3 @@ | |||
{"event_id":"65fad5091a3f4db1b9812777f6dcc052","sent_at":"2023-02-28T16:05:58.058Z","sdk":{"name":"sentry.javascript.nextjs","version":"7.39.0"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep... I think it was included by mistake @priscilawebdev ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that was included by mistake... I am removing and turning this back to "ready to review" 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the exception of the Json file (which I think was added by mistake) everything seems fine.
Please remove the test.json
(unless I completely miss something) then I think it's good to go.
@@ -14,6 +14,12 @@ source: tests/sentry/api/endpoints/test_project_filters.py | |||
hello: localhost - Filter out events coming from localhost | |||
id: localhost | |||
name: Filter out events coming from localhost | |||
- active: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andriisoldatenko @RaduW I used the following command to update this file 😉
SENTRY_SNAPSHOTS_WRITEBACK=1 pytest tests/sentry/api/endpoints/test_project_filters.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I've only looked at the relay part.) Does this come with a PR in relay?
src/sentry/ingest/inbound_filters.py
Outdated
_react_hydration_errors_filter = _FilterSpec( | ||
id=FilterStatKeys.REACT_HYDRATION_ERRORS, | ||
name="Filter out hydration errors", | ||
description="React falls back to do a full re-render on a page and these errors are often not actionable.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we targeting all hydration errors, or only these coming from React?
src/sentry/relay/config/__init__.py
Outdated
# Text content does not match server-rendered HTML. | ||
"https://reactjs.org/docs/error-decoder.html?invariant=425", | ||
] | ||
error_messages = [*error_messages, *react_known_hydration_errors] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error_messages
here may be None
if the option above is not registered, and in that case the unpacking fails.
src/sentry/relay/config/__init__.py
Outdated
"https://reactjs.org/docs/error-decoder.html?invariant=425", | ||
] | ||
error_messages = [*error_messages, *react_known_hydration_errors] | ||
|
||
if error_messages: | ||
filter_settings["errorMessages"] = {"patterns": error_messages} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
c42f48b
to
deacd7d
Compare
Sorry @iker-barriocanal but I just completely changed the logic based on @jan-auer feedback |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #45188 +/- ##
==========================================
- Coverage 79.71% 77.02% -2.69%
==========================================
Files 4724 4720 -4
Lines 198812 198690 -122
Branches 12007 12006 -1
==========================================
- Hits 158482 153050 -5432
- Misses 40068 45378 +5310
Partials 262 262
|
4a6c887
to
167ee74
Compare
frontend #45300 |
src/sentry/relay/config/__init__.py
Outdated
"https://reactjs.org/docs/error-decoder.html?invariant=418", | ||
# The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering. | ||
"https://reactjs.org/docs/error-decoder.html?invariant=419", | ||
# There was an error while hydrating this Suspense boundary. Switched to client rendering. | ||
"https://reactjs.org/docs/error-decoder.html?invariant=422", | ||
# There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering. | ||
"https://reactjs.org/docs/error-decoder.html?invariant=423", | ||
# Text content does not match server-rendered HTML. | ||
"https://reactjs.org/docs/error-decoder.html?invariant=425", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because error_messages
parses glob patterns, we could shorten this to
"https://reactjs.org/docs/error-decoder.html?invariant=418", | |
# The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering. | |
"https://reactjs.org/docs/error-decoder.html?invariant=419", | |
# There was an error while hydrating this Suspense boundary. Switched to client rendering. | |
"https://reactjs.org/docs/error-decoder.html?invariant=422", | |
# There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering. | |
"https://reactjs.org/docs/error-decoder.html?invariant=423", | |
# Text content does not match server-rendered HTML. | |
"https://reactjs.org/docs/error-decoder.html?invariant=425", | |
"https://reactjs.org/docs/error-decoder.html?invariant={418,419,422,423,425}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please leave it like this, obviously more verbose, but the comments are helpful to understand what the numbers actually mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally in favor of keeping the comments, but we could still shorten the value itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I have updated the code shortening the value but still keeping the comments... Hope it's better now
Adds an ingest consumer similar to Replays and Profiles that reads from a Kafka topic `ingest-monitors` and updates them directly in postgres. The consumer runs automatically in devserver and can be started via `sentry run ingest-monitors`. This supports creating and updating check ins with a status and duration. Creating monitors is not supported yet. The consumer closely follows the logic from checkin creation, but there is a difference for disabled monitors compared to the Update (PUT) endpoint.
Problem:
We want to make it clear how Sentry is transforming error payloads, specifically for react-minified errors. This is because it can be hard to tell how to filter these errors (using either beforeSend or server-side inbound filters) as the UI shows the transformed message, not the original one.
Solution:
A new "Inbound Filters" called "Filter out hydration errors" is added via this PR, so users only have to toggle and relay will filter out the most common react hydration errors.
More Details:
Fixes: #45038
Fixes: getsentry/sentry-javascript#6295